Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Button : deprecate isSmall prop #59734

Merged
merged 7 commits into from
Mar 19, 2024
Merged

Button : deprecate isSmall prop #59734

merged 7 commits into from
Mar 19, 2024

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Mar 10, 2024 •
edited
Loading

Closes #59733

What?

This PR officially deprecates the isSmall prop of the Button component.

Why?

#51842 introduced size prop, which covers isSmall prop. #53560 also replaced the isSmall prop with the size prop for Button components throughout the Guenberg project. This should allow us to officially deprecate the isSmall prop.

How?

I added an error indicating that a deprecation warning appears from WP6.6 and will be removed in WP6.9. At the same time, I updated types and unit tests.

Testing Instructions

All CIs should be green. Additionally, use the code below to confirm that a console error is displayed when a Button component with the isSmall prop is registered as a custom block.

wp.blocks.registerBlockType( 'test/button-with-is-small', {
	apiVersion: 3,
	title: 'Button with deprecated isSmall prop',
	edit () {
		return wp.element.createElement(
			'div',
			wp.blockEditor. useBlockProps(),
			wp.element.createElement(
				wp.components.Button,
				{ isSmall: true },
				'Push Me',
			),
		);
	},
} );

deprecated-warning

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components and removed [Type] Enhancement A suggestion for improvement. labels Mar 10, 2024
@t-hamano t-hamano requested a review from a team March 10, 2024 05:03
@t-hamano t-hamano marked this pull request as ready for review March 10, 2024 05:04
@t-hamano t-hamano requested a review from ajitbohra as a code owner March 10, 2024 05:04
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 10, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@t-hamano t-hamano self-assigned this Mar 10, 2024
@mirka mirka mentioned this pull request Mar 15, 2024
Merged
mirka
mirka reviewed Mar 15, 2024
View reviewed changes
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit concerned about the existing deprecation patterns we have in this component, so I'm proposing #59913 to set better conventions for the future. Could you take a look at that please? Then whatever we agree on there, we can integrate those patterns back into this PR.

packages/components/src/button/index.tsx Outdated
deprecated( 'Button isSmall prop', {
since: '6.6',
alternative: 'size="small"',
version: '6.9',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
version: '6.9',

I'm thinking we don't really have to do a hard deprecation on this one, since the maintenance cost is very low. What do you think?

Copy link
Member

@tyxla tyxla Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should start following those more actively and actually clean them all up when they're due. With that in mind, I find them useful and I think they help reduce technical debt over time. scratch that, this comment explained that really well.

@t-hamano
Copy link
Contributor Author

I updated this PR based on #59913. Perhaps the deprecated warning itself is unnecessary?

@mirka mirka added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Mar 18, 2024
mirka
mirka approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

packages/components/src/button/index.tsx Outdated
@@ -49,6 +49,10 @@ function useDeprecatedProps( {
};

if ( isSmall ) {
deprecated( 'Button isSmall prop', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the deprecated warning itself is unnecessary?

Yes, I personally don't think it's worth it in this particular case, but I'm not going to block it if you or others think it's warranted. I will suggest adding a Dev Note for the next WP release though.

If we're going to throw a warning, we should add a prefix:

Suggested change
deprecated( 'Button isSmall prop', {
deprecated( 'wp.components.Button isSmall prop', {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to remove the deprecated() function for now so it doesn't throw any warnings. In the future, we may need to consider adding warnings along with other props such as isPrimary|isTertiary|isSecondary|isLink.

@t-hamano t-hamano removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Mar 19, 2024
@t-hamano t-hamano merged commit 9757688 into trunk Mar 19, 2024
59 checks passed
@t-hamano t-hamano deleted the button/deprecate-is-small branch March 19, 2024 11:56
@github-actions github-actions bot added this to the Gutenberg 18.0 milestone Mar 19, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
@t-hamano @mirka
@tyxla @carstingaxion
Button : deprecate isSmall prop (WordPress#59734)
4b28ddf 
* Button : deprecate `isSmall` prop

* Update changelog

* Update types

* revert unit test changes

* Remove version prop from deprecated function

* Remove `deprecated()` functon

Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyxla tyxla tyxla left review comments

@mirka mirka mirka approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@t-hamano t-hamano

Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 18.0
Development

Successfully merging this pull request may close these issues.

Button: Officially deprecate isSmall prop
3 participants
@t-hamano @mirka @tyxla